Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[qualifier] DSS0210,A2-7-2,7 validate authentication for subscription CRUD+search endpoints #514

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Feb 21, 2024

Adds a scenario to begin validating the DSS0210,A2-7-2,7 requirements:

(The gist of it is that each DSS endpoint should properly check the presence of valid credentials).

This covers the subscription related endpoints (CRUD + search) for the following situations:

  • no credentials provided
  • well-formed but incorrectly signed credentials
  • correct credentials but no provided scope
  • correct credentials but incorrect scope

The other endpoints will be covered in upcoming PRs

@Shastick Shastick force-pushed the dss0210-a7-subs branch 3 times, most recently from de0fc97 to 9f0fbfa Compare February 21, 2024 15:01
@Shastick Shastick marked this pull request as ready for review February 21, 2024 15:20
@Shastick Shastick force-pushed the dss0210-a7-subs branch 3 times, most recently from ec7fc27 to 57e5057 Compare February 22, 2024 10:55
@Shastick
Copy link
Contributor Author

Taking to draft to include some refactors that are needed to support the other entities

@Shastick Shastick marked this pull request as draft February 22, 2024 15:38
@Shastick
Copy link
Contributor Author

Overall structure is now stable: the other entities will be added to the scenario in a way that is similar to how the subscription endpoints are tested.

@Shastick Shastick marked this pull request as ready for review February 22, 2024 17:00
@Shastick Shastick force-pushed the dss0210-a7-subs branch 2 times, most recently from b1ea57a to e8224fd Compare February 22, 2024 17:02
@Shastick Shastick marked this pull request as draft February 23, 2024 11:48
@Shastick Shastick force-pushed the dss0210-a7-subs branch 2 times, most recently from 05ce68e to e3e8674 Compare February 23, 2024 15:42
@Shastick Shastick marked this pull request as ready for review February 23, 2024 15:53
@Shastick Shastick marked this pull request as draft February 28, 2024 17:39
@Shastick Shastick force-pushed the dss0210-a7-subs branch 2 times, most recently from 88d491d to 4daa94f Compare February 29, 2024 09:58
@Shastick Shastick marked this pull request as ready for review February 29, 2024 09:59
@Shastick
Copy link
Contributor Author

Shastick commented Mar 4, 2024

Rebased on top of master: should be ready for review.

"""
super().__init__()
scopes = {Scope.StrategicCoordination: "create and delete subscriptions"}
# We use the AvailabilityArbitration scope as the 'wrong' scope for some tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of necessarily using AvailabilityArbitration, we could just pick a scope that's authorized and not the correct one. I'd probably add a list_authorized_scopes() method to DSSInstanceResource that returns the values in its self._auth_adapter.scopes, then pick one of those that isn't the correct scope and isn't a blank scope (if such an option exists).

"""
q = fetch.query_and_describe(
client=self._invalid_token_session,
scope=Scope.StrategicCoordination,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
scope=Scope.StrategicCoordination,
scope=self._valid_scope,


This scenario will check for the scope's availability and transparently ignore checks that can't be conducted.

The scopes the scenario is expected to be allowed to use are:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It's probably worth clarifying which scopes are mandatory (just utm.strategic_coordination, I think) and which will provide additional test coverage

"""
A scenario that verifies that the DSS properly authenticates requests to all its endpoints.

This scenario does not (yet) cover anything related to authorization: this first version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the distinction wrt authorization -- A2.7.2(7) mentions "authorization" rather than authentication and it seems like we're covering authorization here (e.g., reject for incorrect scope). I would expect authentication in the DSS scope to be mostly limited to checking the signature of the access token, though authentication is one portion of authorization (DSS should generally not authorized unauthenticated users, for instance)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is meant to convey that the scenario is not checking situations where a client that does not own an entity tries to mutate or delete it.

Indeed, checking for scopes is already a step towards authorization checks, and I will reword the comment:

Note that although the wording for A2.7.2(7) contains authorization, I did not interpret it as meaning we need to check all authorization scenarios:

Tests must demonstrate that access to the interfaces is denied when a properly formed authorization with an appropriate authorization scope is not provided.

Here the authorization is something that must be provided by the client (in my assumption), so I interpret that the specification uses that word to refer to the token, not to something that the DSS needs to do. (Authorization checks are or will be properly covered by other scenarios, so an inaccurate interpretation here should have no impact on overall coverage)

Copy link
Member

@BenjaminPelletier BenjaminPelletier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good following these comments

# For the 'wrong' scope we pick anything from the available scopes that isn't the SCD or empty scope:
available_scopes = dss.get_authorized_scopes()
available_scopes.remove(Scope.StrategicCoordination)
available_scopes.remove("")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw a KeyError if the empty string is not present in available_scopes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced .remove with discard

scopes = {Scope.StrategicCoordination: "create and delete subscriptions"}
# For the 'wrong' scope we pick anything from the available scopes that isn't the SCD or empty scope:
available_scopes = dss.get_authorized_scopes()
available_scopes.remove(Scope.StrategicCoordination)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw a KeyError if the scope isn't in available_scopes. We haven't yet made the call to get_instance, so we don't yet know if this scope is actually available or not. After calling get_instance, either we'll have a working DSSInstance and can be confident the required scopes are available, or else a MissingResourceError would have aborted execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, indeed. get_instance is now called at the earliest possible time in the constructor.

available_scopes.discard("")

self._wrong_scope = (
random.choice(list(available_scopes)) if available_scopes else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we want automated tests to be repeatable. Pseudorandomness is ok, but we should generally initialize any pseudorandom generator with a fixed seed so test results are repeatable. In this case, we probably shouldn't pick an item randomly, but instead select whatever item is "first".

scopes = {Scope.StrategicCoordination: "create and delete subscriptions"}
# Note: .get_instance needs to be called before .get_authorized_scopes to
# guarantee that the returned scopes are available for use.
self._dss = dss.get_instance(scopes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line indicates that self._dss will only ever make calls with scopes, which only contains StrategicCoordination at this point. It seems like we should wait to call this until we know what scopes we want to use (i.e., ~line 84)

random.choice(list(available_scopes)) if available_scopes else None
)
if self._wrong_scope:
scopes[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not doing anything here because scopes was already used above to get_instance. get_instance should be called after we have figured out all our scopes.

"""
super().__init__()
scopes = {Scope.StrategicCoordination: "create and delete subscriptions"}
# Note: .get_instance needs to be called before .get_authorized_scopes to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_authorized_scopes can be called any time (no need to call get_instance first), and we want to use it to figure out what scopes we need in order to be able to call get_instance with the appropriate information.

@BenjaminPelletier BenjaminPelletier merged commit 46c020a into interuss:main Mar 19, 2024
9 checks passed
github-actions bot added a commit that referenced this pull request Mar 19, 2024
… CRUD+search endpoints (#514)

* DSS02120,A2-7-2,7

Implement comments

* Address latest PR comments 46c020a
@Shastick Shastick deleted the dss0210-a7-subs branch March 19, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants